Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not roll back rejected-promise navigations #60

Merged
merged 1 commit into from
Mar 5, 2021
Merged

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Mar 5, 2021

This removes some of the complexity introduced in 9d750db. Closes #47.

This removes some of the complexity introduced in 9d750db. Closes #47.
@frehner
Copy link

frehner commented Mar 5, 2021

Ah, I think this mostly answers a question I've had:

Given the following situation with multiple navigate event handlers:

appHistory.addEventListener('navigate', e => {
  e.respondWith(Promise.reject())
})

appHistory.addEventListener('navigate', e => {
  e.respondWith((async () => {
    doPotentiallyExpensiveWork()
    // for API calls there is `e.signal`, so that case is covered
  }))
})

I had been wondering if the second event handler could somehow get notified that the navigation was cancelled and avoid calling doPotentiallyExpensiveWork()

But if I understand this change correctly, a rejected promise in respondWith will still navigate to the url; so doPotentiallyExpensiveWork() is still useful there (as opposed to how the spec, before this PR, was to "roll back" the navigation and render the work done in doPotentiallyExpensiveWork() useless).

Does that sound correct?

@domenic
Copy link
Collaborator Author

domenic commented Mar 5, 2021

Yeah, that's another good point in favor of this less-complicated design! Although I'll admit that generally dealing with multiple concurrent or queued-up navigations is still a bit up in the air; we have some idea that signal will cancel ongoing ones, but the issues in #10, #11, and #13 are not yet resolved.

@domenic domenic merged commit 864629c into main Mar 5, 2021
@domenic domenic deleted the rm-rollback branch March 5, 2021 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handling failed single-page app navs
2 participants